Not for this PR, but we should create some standard way to handle these options (noCache is also related).

Other places where a similar mechanism exists:

• the linter, which essentially defaults to auto
• runtime-version, which uses onebehindfetch
• the validator (defaults to network)
• … probably others

Migrated this to its own issue: #378

FWIW: this is the only toolbox package that works with validator.json (as opposed to validator.js).

In theory, this is meant to be a low-level interface to that file, so no other tool should ever need to load validator.json directly.

+1 to Michael's comment

Well, it's not obvious from the log, but if I try to make a test break (e.g. change one expected result), npm test complains, so I'm quite confident it runs.

The pattern also matches this file if I'm reading it correctly (** should also work for .)?

You're right, comment withdrawn!

Can we use a more descriptive name? Suggestion:

import { ValidationRules } from '@ampproject/toolbox-validator-query';

const validationRules = await ValidationRules.fetch()

Hm, I'm OK with renaming, but I have two problems with that:

• fetch sounds like it misrepresents what that method does since it may not fetch anything, depending on the options provided.
• What other methods would ValidationRules have? I'm kind of reluctant to expose the AmpValidatorQuery class itself since the end user shouldn't need to instantiate it on their own and I don't see any other functions we'd need. That said, I'm OK with having ValidationRules an object with a single method if you feel that makes sense.

The reason I prefer to export a class is that it becomes easier to test:

Users will use:

const validationRules = await ValidationRules.fetch()

For testing you can do:

const validationRules = new ValidationRules(fetchMock).fetch()

fetch was just a suggestion. What I like about it is that it makes it clear that this might perform a network request (as opposed to ValidationRules.get().

So, I don't see a reason to make the class public since testing is already possible by directly including (see AmpValidatorRulesSpec.js) - so this is the closest we can get to package-private constructor. The user can also specify a mock to the load function via the rules parameter (e.g. load({ rules: { ... } })). Exposing the class makes me feel like it can cause a lot of confusion.

Even better if we don't need to export a class, however, we still should export an object in that case:

module.exports = {
  fetch: ... (or whatever name we decide on)
}

This is to keep the possibility to add more methods in the future and to be consistent with the other modules, e.g.:

const AmpOptimizer = require('@ampproject/toolbox-optimizer');

const ampOptimizer = AmpOptimizer.create();

Done, renamed to fetch and ensured it's exported as an object (it was already, but I made it clear in the readme).

+1 to Michael's comment

We should move this functionality into the core package (we need it for optimizer as well) and find a common approach. As a start, I'd suggest only using OneBehindFetch and add caching later.

Fixed, sort of. I consider the current loadRules.js temporary, so when we figure out what to do with #378, most of it will probably go away.

The unit test doesn't do any network requests - in fact, the AmpValidatorQuery can't fetch the rules, it expects them to be provided (load fetches them and passes them to AmpValidatorQuery). Here the rules are provided as a local object.

You're right, I was one step ahead in my mind.

Fixed

Linked to relevant proto definitions and added examples for all methods.

Add JSDoc

Add JSDoc

I'd expect this method to return an array. Why return an object instead?

• There shouldn't ever be more than one extension with the same name and...
• ...I assumed the query would usually be "give me information about amp-bind", so an object seems more appropriate. Also, it's cheap to transform an object back into an array with Object.keys and/or Object.values.

How about turning this into two different APIs?

const ampBindRule = validationRules.getExtension('amp-bind');

const extensions = validationRules.getExtensions() <- returns an array containing all extensions
const mailExtensions = validationRules.getExtensions({format: 'AMP4EMAIL'}) <- with optional format parameter

I removed getExtensions and instead exposed it as an extensions property and added getExtension which follows your suggestion.

Looking back, I wonder if I can just reuse checkEntityFormat_ and call it as !this.checkEntityFormat_(entity, 'transformed')...

Will need to give it some thought.

Why do you need to provide the format here? This seems counterintuitive.

This is just how validator rules are written - there's more than one extension definition with the same name, but different formats.

The implementation uses a different order getExtension(format, extension).

Ouch, good catch. Fixed.

I don't even remember why I had it there. Thanks for flagging, removed.

Instead of configuring this via options, you could also provide to methods fetch and loadFile on the ValidationRules object.

I feel the functionality is not different enough to vouch having two different functions and that also prevents me from easily having the default option which is "try local if possible".

That said, maybe source is not needed if the url is a local file, i.e. use something like

Ideally, whatever the resolution of #378 ends up being will replace this file anyway.

Sounds good. I'd suggest removing loadLocal until we settled on a common approach. Once it's out there it's harder to deprecate and I don't think that the feature is critical.

Fair point, done.

nit: turn this into a link

Don't think that makes sense, clicking that link is probably not very useful, it's more meaningful as an identifier.

I'd want to click it :-). But I agree unformatted JSON is not helpful.

nit: s/atrrs/attrLists

Your comment explains what you're doing, but I get that from the code. I'm more interested in the why (which I don't understand)

Tried rewriting. The key thing is that attrLists contains a list of IDs that are looked up from the top-level attrLists property. It doesn't contain any real attribute definition. This bit of code merges the real attribute definitions (from the global object) into attrs.

Let me know if the current comment makes sense to convey that.

Sounds good. I'd suggest removing loadLocal until we settled on a common approach. Once it's out there it's harder to deprecate and I don't think that the feature is critical.

The implementation uses a different order getExtension(format, extension).

I'd want to click it :-). But I agree unformatted JSON is not helpful.

I learned something, was't aware that restructuring works for params.